archive: harden tar extraction against path traversal - #45
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #45 +/- ##
==========================================
- Coverage 65.81% 64.19% -1.63%
==========================================
Files 42 44 +2
Lines 2039 2223 +184
==========================================
+ Hits 1342 1427 +85
- Misses 519 594 +75
- Partials 178 202 +24 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
979434e to
fc27b1b
Compare
|
Thanks for the detailed review @thaJeztah — I've reworked the PR to the native-at-boundary model you laid out (and squashed it to a single commit, plus a separate tests commit):
Also added regression tests that fail on the pre-fix code and pass here (parent- |
|
@ctalledo can you fix the linting failure so that CI can run? (and perhaps the small nit I left if you're updating anyway) |
- unrepresentableOnWindows now returns an error naming which value (entry name or hardlink target) is unrepresentable, so the skip log is accurate instead of always printing hdr.Name. - Use strings.SplitSeq (go1.25) in createImpliedDirectories to avoid the intermediate slice allocation. - makeRootPathTest: drop the unused *testing.T parameter to stay close to the containerd/continuity upstream. Addresses review feedback from thaJeztah on moby#45. Signed-off-by: Cesar Talledo <cesar.talledo@docker.com>
|
Done in 7677303, @thaJeztah:
CI is green on the updated head. Thanks for the review! |
There was a problem hiding this comment.
Pull request overview
This PR consolidates prior work to harden tar extraction against path traversal by bounding extraction operations with os.Root (kernel-enforced containment), rejecting non-local entry names (absolute / ..), and improving runtime safety around symlink/hardlink handling. It also introduces a directory-FD cache to reduce repeated path-walk overhead during extraction and adds regression tests for previously exploitable breakout cases.
Changes:
- Switch tar extraction (
Unpack,UnpackLayer,createTarFile) toos.Root-bounded filesystem operations and reject non-local entry names. - Add
fsRootPath(forked fromcontainerd/continuity) plus tests to safely resolve paths through symlinks within a root. - Add
dirCacheto reuse the last parent directory FD (Unix) and update/extend regression tests for traversal and symlink breakout cases.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| utils_test.go | Updates breakout test walk behavior to skip symlinks under the os.Root security model. |
| time_windows.go | Removes legacy lchtimes helper (now handled via dirCache). |
| time_nonwindows.go | Removes legacy lchtimes helper (now handled via dirCache). |
| rootpath.go | Adds fsRootPath implementation for symlink-aware, root-bounded path resolution. |
| rootpath_test.go | Adds non-Windows unit tests for fsRootPath. |
| go.mod | Adds github.com/containerd/continuity dependency for tests/utilities. |
| go.sum | Records new module checksums (continuity + indirect deps). |
| dircache_windows.go | Introduces Windows dirCache implementation (delegates to os.Root). |
| dircache_unix.go | Introduces Unix dirCache implementation using *at(2) syscalls for perf. |
| diff.go | Moves UnpackLayer extraction to os.Root, integrates dirCache, updates whiteout handling. |
| chrootarchive/archive_unix_test.go | Loosens assertion to accept either bounded-failure point while still verifying safety. |
| archive.go | Core extraction hardening: os.Root, non-local rejection, new createTarFile signature, implied dir creation changes, Windows-name filtering. |
| archive_windows.go | Updates handleLChmod signature to match new extraction flow (no-op on Windows). |
| archive_unix.go | Updates handleLChmod to use dirCache/os.Root for bounded chmod. |
| archive_test.go | Updates tests for the new createTarFile signature and adds new regression tests. |
Comments suppressed due to low confidence (1)
utils_test.go:154
- filepath.WalkDir callback calls info.IsDir() before checking err. When WalkDir reports an error, info can be nil, which can panic the test (nil dereference) instead of cleanly skipping the entry.
return filepath.WalkDir(dest, func(path string, info os.DirEntry, err error) error {
if info.IsDir() {
if err != nil {
// skip directory if error
return filepath.SkipDir
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
272b3be to
86471f1
Compare
- unrepresentableOnWindows now returns an error naming which value (entry name or hardlink target) is unrepresentable, so the skip log is accurate instead of always printing hdr.Name. - Use strings.SplitSeq (go1.25) in createImpliedDirectories to avoid the intermediate slice allocation. - makeRootPathTest: drop the unused *testing.T parameter to stay close to the containerd/continuity upstream. Addresses review feedback from thaJeztah on moby#45. Signed-off-by: Cesar Talledo <cesar.talledo@docker.com>
86471f1 to
4774927
Compare
| // created within root via openat(2) semantics, without resolving to an | ||
| // absolute path; containment applies when the symlink is followed, not | ||
| // at creation. | ||
| if err := root.Symlink(filepath.FromSlash(hdr.Linkname), dstPath); err != nil { |
There was a problem hiding this comment.
Actually looking at this one, and this may be wrong ... for symlinks, the target of the symlink is part of the symlink's content, so we need to preserve that verbatim, even if that doesn't match the platform's semantics.
So
root.Symlink("/usr/local/bin", "somewhere/in/root") should stay /usr/local/bin even if that doesn't match Windows.
It's a slightly tricky one perhaps in some situations, because that would produce either a broken symlink, or a symlink pointing outside the exported data, but it preserves the data as it was provided.
This comment was marked as resolved.
This comment was marked as resolved.
- unrepresentableOnWindows now returns an error naming which value (entry name or hardlink target) is unrepresentable, so the skip log is accurate instead of always printing hdr.Name. - Use strings.SplitSeq (go1.25) in createImpliedDirectories to avoid the intermediate slice allocation. - makeRootPathTest: drop the unused *testing.T parameter to stay close to the containerd/continuity upstream. Addresses review feedback from thaJeztah on moby#45. Signed-off-by: Cesar Talledo <cesar.talledo@docker.com>
816698c to
9849cef
Compare
- unrepresentableOnWindows now returns an error naming which value (entry name or hardlink target) is unrepresentable, so the skip log is accurate instead of always printing hdr.Name. - Use strings.SplitSeq (go1.25) in createImpliedDirectories to avoid the intermediate slice allocation. - makeRootPathTest: drop the unused *testing.T parameter to stay close to the containerd/continuity upstream. Addresses review feedback from thaJeztah on moby#45. Signed-off-by: Cesar Talledo <cesar.talledo@docker.com>
| // isPathEscapes reports whether err is os.Root's path-containment error. | ||
| // | ||
| // os.Root currently returns an unexported errPathEscapes sentinel, so callers | ||
| // cannot detect it with errors.Is. Keep the string comparison isolated here | ||
| // until Go exports the error; see https://go.dev/issue/74640. | ||
| func isPathEscapes(err error) bool { | ||
| // https://github.com/golang/go/blob/go1.26.5/src/os/file.go#L421 | ||
| const errPathEscapes = "path escapes from parent" | ||
| for err != nil { | ||
| if errors.Unwrap(err) == nil { | ||
| return err.Error() == errPathEscapes | ||
| } | ||
| err = errors.Unwrap(err) | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Sigh; good'Ol string-matching needed to match this;
| path = newpath | ||
| if i == linksWalked { | ||
| newpath = filepath.Join("/", newpath) | ||
| newpath = filepath.Join(string(os.PathSeparator), newpath) |
There was a problem hiding this comment.
Opened a PR in upstream to fix it there as well;
|
Does this LGTY? |
|
Rebased; without vvoland@7f858a5 - CI should fail, and will push that patch after. |
|
Yup; fails; that's good |
Co-authored-by: Cesar Talledo <cesar.talledo@docker.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Addresses ART-224 and the cluster of externally reported tar-extraction
breakouts (Windows BuildKit ADD/build, and docker cp on all platforms).
- Reject traversal entries instead of clamping them: normalize hdr.Name
with path.Clean(strings.TrimLeft(name, "/")) and reject non-local names
via filepath.IsLocal, in both Unpack and UnpackLayer.
- Bound extraction with os.Root (openat-based); create symlinks with
root.Symlink (target stored verbatim, so absolute targets are kept) and
hardlinks with root.Link plus a filepath.IsLocal defence-in-depth check.
- Cache the most recent parent directory fd (dirCache) so consecutive
entries in the same directory use *at(2) syscalls, amortizing os.Root's
per-call path re-evaluation.
- Resolve symlink components with fsRootPath, a straight fork of
containerd/continuity fs.RootPath (path.go + path_test.go), un-exported
and trimmed to the functions used, to ease upstream sync.
- tar header names are POSIX; convert to native paths with
filepath.FromSlash at each os.Root / filesystem boundary, and skip
entries whose name or hardlink target Windows cannot represent (":", "\").
archive: make lchtimes use os.Root for path resolution
Resolve the parent directory through os.Root and perform utimensat(2)
relative to the opened directory instead of using an absolute host path.
This preserves os.Root's path containment guarantees while still updating
the symlink itself using AT_SYMLINK_NOFOLLOW.
createImpliedDirectories: Keep implied dirs at ImpliedDirectoryMode under umask
createImpliedDirectories previously used user.MkdirAllAndChown, whose
setPermissions runs os.Chmod after creation, so implied parent
directories always ended up with ImpliedDirectoryMode regardless of the
process umask.
The os.Root rewrite creates them with root.Mkdir only, which applies the
mode subject to umask: under umask 0o027 an implied directory became
0o750 instead of 0o755.
Re-apply the mode with root.Chmod after each successful Mkdir so implied
directories keep ImpliedDirectoryMode independent of umask, matching the
prior behavior and the function's documented contract.
Co-authored-by: Cesar Talledo <cesar.talledo@docker.com>
Co-authored-by: Paweł Gronowski <git@grono.dev>
Co-authored-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Cesar Talledo <cesar.talledo@docker.com>
Signed-off-by: Paweł Gronowski <git@grono.dev>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
os.Root.Chmod relies on chmodat(AT_SYMLINK_NOFOLLOW), which is not supported on all Linux kernels and filesystems. When that operation fails with ENOTSUP or EOPNOTSUPP, fall back to chmod relative to the resolved parent directory. Symlink entries are excluded beforehand, and hardlink entries are only chmod'd when their target is not a symlink, preserving the existing no-follow semantics. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Some code-paths may return an error, in which case creating the parent paths isn't needed. Move it later in the function to avoid this. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Open newly-created implied directories once and apply ownership and permissions through the retained handle instead of separate os.Root operations. This avoids repeated doInRoot path resolution for Lchown/Chmod while continuing to apply metadata only to newly-created directories. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Pass the extraction root to the overlay whiteout converter and operate relative to opened directories instead of resolving absolute paths with fsRootPath. This avoids redundant path resolution before os.Root-based operations while reducing the TOCTOU surface. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Pass os.Root into handleTarTypeBlockCharFifo and perform the filesystem operation relative to the opened parent directory instead of constructing an absolute path. This removes an fsRootPath call, avoids an extra pathname resolution, and keeps the operation within os.Root. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
|
I'll bring this one in; we may have some follow-ups, but this has had enough eyes by now 😅 |
Combines the tar path-traversal hardening previously split across #24, #25 and #26 into a single PR, rebased onto current
main. Addresses ART-224 and the externally-reported tar-extraction breakouts (Windows BuildKitADD/build, anddocker cpon all platforms).What it does
.., absolute) instead of clamping them, and bound all extraction withos.Root(openat-based) for kernel-enforced containment.root.Symlink(target stored verbatim, so absolute targets are preserved) and hardlinks withroot.Link+ afilepath.IsLocalcheck.fsRootPath, a straight fork ofcontainerd/continuityfs.RootPath(kept verbatim, with its test, for easy upstream sync).filepath.FromSlash) at each filesystem boundary; skip entries whose name/target Windows cannot represent.dirCache— reuse the parent-directory fd across consecutive entries (*at(2)syscalls) to amortizeos.Root's per-call path re-evaluation.Includes regression tests that fail on the previous code. Supersedes #24, #25 and #26. Windows-specific test coverage needs Windows CI and is a follow-up.
cc @thaJeztah @vvoland @tonistiigi